feat: ingress configurations + external secrets#35
feat: ingress configurations + external secrets#35jsbroks merged 7 commits intoctrlplanedev:mainfrom
Conversation
zachspar
requested review from
adityachoudhari26,
jsbroks and
zacharyblasczyk
as code owners
zachspar
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds conditional valueFrom support and reuse logic for encryption and auth secrets across Helm charts; guards secret creation when external secrets are used; enhances ingress with fqdn extraction, optional TLS/ingressClassName, and updates chart values and version. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
charts/ctrlplane/charts/api/templates/secrets.yaml (1)
1-17: Minor inconsistency withcharts/ctrlplane/templates/secrets.yaml.Two small differences compared to the parent chart's encryption-key secret template:
- Missing
type: Opaque— The parent chart'ssecrets.yamlincludestype: Opaque. While omitting it is functionally equivalent (Kubernetes defaults to Opaque), it's inconsistent.| quoteon generated value — The parent chart uses{{ randAlphaNum 64 | b64enc | quote }}while this template (andweb/secrets.yaml) omit| quote. Both work, but should be consistent.Consider aligning both secret templates with the parent chart for maintainability.
Suggested alignment
apiVersion: v1 kind: Secret metadata: name: {{ $secretName }} labels: {{- include "api.labels" . | nindent 4 }} +type: Opaque data: {{- if and $existing (hasKey $existing.data $secretKey) }} {{ $secretKey }}: {{ index $existing.data $secretKey }} {{- else }} - {{ $secretKey }}: {{ randAlphaNum 64 | b64enc }} + {{ $secretKey }}: {{ randAlphaNum 64 | b64enc | quote }} {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/ctrlplane/charts/api/templates/secrets.yaml` around lines 1 - 17, The Secret template omits the explicit "type: Opaque" and doesn't quote the base64 values, causing inconsistency with the parent chart; update the template that builds the Secret (the block using $secretName, $secretKey and $existing) to include a "type: Opaque" field under metadata and apply | quote to the base64 value usages so both branches use {{ index $existing.data $secretKey | quote }} and {{ randAlphaNum 64 | b64enc | quote }}; make the same change in the sibling web/secrets.yaml to keep templates consistent.charts/ctrlplane/templates/ingress.yaml (1)
2-2: Consider also trimming a trailing slash fromfqdn.If a user sets
global.fqdntohttps://example.com/, the result aftertrimPrefixwould beexample.com/, which is not a valid ingress hostname. A defensivetrimSuffix "/"would prevent this edge case.Suggested fix
-{{- $fqdn := .Values.global.fqdn | trimPrefix "https://" | trimPrefix "http://" -}} +{{- $fqdn := .Values.global.fqdn | trimPrefix "https://" | trimPrefix "http://" | trimSuffix "/" -}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/ctrlplane/templates/ingress.yaml` at line 2, The fqdn pipeline that assigns $fqdn currently uses trimPrefix to strip "https://" and "http://" but doesn't remove a trailing slash, so update the pipeline that defines $fqdn (the expression setting $fqdn in ingress.yaml) to also trimSuffix "/" (e.g. add | trimSuffix "/" to the chain) so values like "https://example.com/" become "example.com" for a valid ingress hostname.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@charts/ctrlplane/charts/web/templates/secrets.yaml`:
- Around line 1-17: The template for authSecret currently defaults $secretName
to include "web.fullname", causing a mismatch with the API chart which uses
"api.fullname"; update the default to use the same identifier as the API chart
(e.g., replace include "web.fullname" with include "api.fullname" or a shared
helper) so that $secretName resolves the same when
.Values.global.secrets.authSecret.secretName is empty; ensure references to
$secretName, authSecret, and the include helper are adjusted consistently so
both charts point to the same secret name.
---
Nitpick comments:
In `@charts/ctrlplane/charts/api/templates/secrets.yaml`:
- Around line 1-17: The Secret template omits the explicit "type: Opaque" and
doesn't quote the base64 values, causing inconsistency with the parent chart;
update the template that builds the Secret (the block using $secretName,
$secretKey and $existing) to include a "type: Opaque" field under metadata and
apply | quote to the base64 value usages so both branches use {{ index
$existing.data $secretKey | quote }} and {{ randAlphaNum 64 | b64enc | quote }};
make the same change in the sibling web/secrets.yaml to keep templates
consistent.
In `@charts/ctrlplane/templates/ingress.yaml`:
- Line 2: The fqdn pipeline that assigns $fqdn currently uses trimPrefix to
strip "https://" and "http://" but doesn't remove a trailing slash, so update
the pipeline that defines $fqdn (the expression setting $fqdn in ingress.yaml)
to also trimSuffix "/" (e.g. add | trimSuffix "/" to the chain) so values like
"https://example.com/" become "example.com" for a valid ingress hostname.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
charts/ctrlplane/charts/api/templates/deployment.yaml (2)
52-56: Consider adding| quoteto rendered secret name/key values.If a user-supplied
secretNameorsecretKeyvalue contains YAML-special characters (e.g.,:,{), the unquoted output would break YAML parsing. The defaults (printf "%s-encryption-key" .Release.Nameand"AES_256_KEY") are safe, but external user values are not guaranteed to be.🛡️ Proposed fix
- name: {{ .Values.global.secrets.encryptionKey.secretName | default (printf "%s-encryption-key" .Release.Name) }} - key: {{ .Values.global.secrets.encryptionKey.secretKey | default "AES_256_KEY" }} + name: {{ .Values.global.secrets.encryptionKey.secretName | default (printf "%s-encryption-key" .Release.Name) | quote }} + key: {{ .Values.global.secrets.encryptionKey.secretKey | default "AES_256_KEY" | quote }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/ctrlplane/charts/api/templates/deployment.yaml` around lines 52 - 56, The YAML output for secretKeyRef under the env var VARIABLES_AES_256_KEY can break if user-supplied .Values.global.secrets.encryptionKey.secretName or .Values.global.secrets.encryptionKey.secretKey contain YAML-special characters; update the template so the rendered secret name and key are piped through the Helm quote function (add | quote) where .Values.global.secrets.encryptionKey.secretName and .Values.global.secrets.encryptionKey.secretKey are used (inside the secretKeyRef block for VARIABLES_AES_256_KEY) to ensure the values are properly quoted in the generated deployment.yaml.
71-75: Same| quotegap as the encryption key block above.Same reasoning applies: user-supplied
authSecret.secretNameorauthSecret.secretKeyvalues aren't protected against YAML-special characters.🛡️ Proposed fix
- name: {{ .Values.global.secrets.authSecret.secretName | default (include "api.fullname" .) }} - key: {{ .Values.global.secrets.authSecret.secretKey | default "AUTH_SECRET" }} + name: {{ .Values.global.secrets.authSecret.secretName | default (include "api.fullname" .) | quote }} + key: {{ .Values.global.secrets.authSecret.secretKey | default "AUTH_SECRET" | quote }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/ctrlplane/charts/api/templates/deployment.yaml` around lines 71 - 75, The AUTH_SECRET env var block uses unquoted Helm template expansions for the secret name and key (.Values.global.secrets.authSecret.secretName and .Values.global.secrets.authSecret.secretKey with defaults including (include "api.fullname" .) and "AUTH_SECRET"); wrap both template expressions with the Helm | quote function so the rendered YAML escapes any special characters (i.e., apply | quote to the name and to the key defaults in the AUTH_SECRET secretKeyRef).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@charts/ctrlplane/charts/api/templates/deployment.yaml`:
- Around line 52-56: The YAML output for secretKeyRef under the env var
VARIABLES_AES_256_KEY can break if user-supplied
.Values.global.secrets.encryptionKey.secretName or
.Values.global.secrets.encryptionKey.secretKey contain YAML-special characters;
update the template so the rendered secret name and key are piped through the
Helm quote function (add | quote) where
.Values.global.secrets.encryptionKey.secretName and
.Values.global.secrets.encryptionKey.secretKey are used (inside the secretKeyRef
block for VARIABLES_AES_256_KEY) to ensure the values are properly quoted in the
generated deployment.yaml.
- Around line 71-75: The AUTH_SECRET env var block uses unquoted Helm template
expansions for the secret name and key
(.Values.global.secrets.authSecret.secretName and
.Values.global.secrets.authSecret.secretKey with defaults including (include
"api.fullname" .) and "AUTH_SECRET"); wrap both template expressions with the
Helm | quote function so the rendered YAML escapes any special characters (i.e.,
apply | quote to the name and to the key defaults in the AUTH_SECRET
secretKeyRef).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/ctrlplane/charts/workspace-engine/templates/statefulset.yaml (1)
46-50:⚠️ Potential issue | 🔴 Critical
AES_256_KEYwill reference a non-existent secret when using external secrets — pod crash.
templates/secrets.yamlis now guarded by{{- if not (include "ctrlplane.isValueFrom" .Values.global.secrets.encryptionKey) }}. When an external secret is configured,{{ .Release.Name }}-encryption-keyis never created, but this env var still unconditionally references it. The workspace-engine pod will fail to start with anError: secret "...-encryption-key" not foundevent.
VARIABLES_AES_256_KEYat lines 74–84 is already guarded correctly;AES_256_KEYneeds the same treatment.🐛 Proposed fix
- - name: AES_256_KEY - valueFrom: - secretKeyRef: - name: {{ .Release.Name }}-encryption-key - key: AES_256_KEY + {{- if include "ctrlplane.isValueFrom" .Values.global.secrets.encryptionKey }} + - name: AES_256_KEY + valueFrom: + {{- toYaml .Values.global.secrets.encryptionKey.valueFrom | nindent 16 }} + {{- else }} + - name: AES_256_KEY + valueFrom: + secretKeyRef: + name: {{ .Release.Name }}-encryption-key + key: AES_256_KEY + {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/ctrlplane/charts/workspace-engine/templates/statefulset.yaml` around lines 46 - 50, The AES_256_KEY env var in workspace-engine statefulset unconditionally references {{ .Release.Name }}-encryption-key which is not created when using external secrets; wrap the AES_256_KEY env var block in the same guard used for templates/secrets.yaml (the conditional that checks include "ctrlplane.isValueFrom" .Values.global.secrets.encryptionKey) so the AES_256_KEY entry is only emitted when the chart is creating the secret (mirror the guard used around VARIABLES_AES_256_KEY).
🧹 Nitpick comments (1)
charts/ctrlplane/tests/secrets_test.yaml (1)
2-6: Missing deployment templates forwebandpty-proxyblock wiring test coverage.
template:used at the test job level must be included in the suite-leveltemplates:list — omittingcharts/web/templates/deployment.yamlandcharts/pty-proxy/templates/deployment.yamlmakes it impossible to write wiring assertions for those components in this suite.The PR modifies
pty-proxy/templates/deployment.yamlto conditionally sourceVARIABLES_AES_256_KEYandAUTH_SECRETfrom external secrets, yet there are zero tests verifying that wiring. The same gap exists for the web deployment'sAUTH_SECRETenv binding.🧪 Proposed fix — add the missing templates and representative wiring tests
templates: - templates/secrets.yaml - charts/api/templates/secrets.yaml - charts/web/templates/secrets.yaml - charts/api/templates/deployment.yaml + - charts/web/templates/deployment.yaml + - charts/pty-proxy/templates/deployment.yamlThen add test jobs such as:
- it: points the web at the auto-generated auth secret template: charts/web/templates/deployment.yaml asserts: - contains: path: spec.template.spec.containers[0].env content: name: AUTH_SECRET valueFrom: secretKeyRef: name: ctrlplane-web key: AUTH_SECRET any: true - it: wires the web to an external auth secret via valueFrom template: charts/web/templates/deployment.yaml set: global: secrets: authSecret: valueFrom: secretKeyRef: name: my-ext-auth key: MY_AUTH asserts: - contains: path: spec.template.spec.containers[0].env content: name: AUTH_SECRET valueFrom: secretKeyRef: name: my-ext-auth key: MY_AUTH any: true - it: points pty-proxy at the auto-generated encryption-key secret template: charts/pty-proxy/templates/deployment.yaml asserts: - contains: path: spec.template.spec.containers[0].env content: name: VARIABLES_AES_256_KEY valueFrom: secretKeyRef: name: ctrlplane-encryption-key key: AES_256_KEY any: true - it: wires pty-proxy to an external encryption key via valueFrom template: charts/pty-proxy/templates/deployment.yaml set: global: secrets: encryptionKey: valueFrom: secretKeyRef: name: my-ext-aes key: MY_AES asserts: - contains: path: spec.template.spec.containers[0].env content: name: VARIABLES_AES_256_KEY valueFrom: secretKeyRef: name: my-ext-aes key: MY_AES any: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/ctrlplane/tests/secrets_test.yaml` around lines 2 - 6, The suite-level templates list in charts/ctrlplane/tests/secrets_test.yaml is missing charts/web/templates/deployment.yaml and charts/pty-proxy/templates/deployment.yaml, so add those two entries to the top-level templates: array; then add test jobs that target those templates to assert the env wiring: for charts/web/templates/deployment.yaml add tests asserting container env contains name: AUTH_SECRET bound via valueFrom.secretKeyRef to the auto-generated ctrlplane-web/AUTH_SECRET and a second test that sets global.secrets.authSecret.valueFrom (e.g., name: my-ext-auth, key: MY_AUTH) and asserts the ARTIFACT uses that secret; for charts/pty-proxy/templates/deployment.yaml add tests asserting VARIABLES_AES_256_KEY is wired to ctrlplane-encryption-key/AES_256_KEY and a counterpart that sets global.secrets.encryptionKey.valueFrom (e.g., my-ext-aes/MY_AES) and asserts the valueFrom binding is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/ctrlplane/charts/web/templates/secrets.yaml`:
- Around line 11-15: The AUTH_SECRET generation is inconsistent: when reusing an
existing Secret you assign the base64 value via index $existing.data
"AUTH_SECRET", but the newly-generated branch uses {{ randAlphaNum 64 | b64enc
}} without | quote; update the generated branch to mirror the existing-template
style by piping the b64 output through | quote (i.e., change the randAlphaNum 64
| b64enc branch to include | quote) so both branches produce consistent YAML for
AUTH_SECRET.
In `@charts/ctrlplane/templates/secrets.yaml`:
- Around line 13-16: The AES_256_KEY value paths are inconsistently quoted — the
reuse path uses index $existing.data "AES_256_KEY" without | quote while the
generate path uses randAlphaNum 64 | b64enc | quote; make them consistent by
applying the same quoting style to both branches (either add | quote to the
reuse branch that uses index $existing.data "AES_256_KEY" or remove | quote from
the generate branch), updating the conditional block around AES_256_KEY so both
the reuse and generate expressions use the same pipeline (refer to AES_256_KEY,
index $existing.data "AES_256_KEY", randAlphaNum, b64enc, and quote).
In `@charts/ctrlplane/values.yaml`:
- Around line 131-133: The default of tls.enabled is currently true which can
introduce a spec.tls entry on upgrades for users who only set global.fqdn;
change the default to false so TLS is opt-in: update the tls block in
values.yaml (the tls.enabled key) to false (and keep secretName behavior
unchanged), ensuring existing deployments without a TLS secret won’t get an
unexpected spec.tls on helm upgrade.
---
Outside diff comments:
In `@charts/ctrlplane/charts/workspace-engine/templates/statefulset.yaml`:
- Around line 46-50: The AES_256_KEY env var in workspace-engine statefulset
unconditionally references {{ .Release.Name }}-encryption-key which is not
created when using external secrets; wrap the AES_256_KEY env var block in the
same guard used for templates/secrets.yaml (the conditional that checks include
"ctrlplane.isValueFrom" .Values.global.secrets.encryptionKey) so the AES_256_KEY
entry is only emitted when the chart is creating the secret (mirror the guard
used around VARIABLES_AES_256_KEY).
---
Duplicate comments:
In `@charts/ctrlplane/charts/api/templates/secrets.yaml`:
- Around line 11-15: The AUTH_SECRET block in api/templates/secrets.yaml uses
the same pattern as web/templates/secrets.yaml but the generated or injected
secret value isn't wrapped with | quote; update the template so the branch that
outputs the existing secret (index $existing.data "AUTH_SECRET") and the branch
that generates a new secret (randAlphaNum 64 | b64enc) are both piped to | quote
to match the style in templates/secrets.yaml and avoid YAML parsing issues.
---
Nitpick comments:
In `@charts/ctrlplane/tests/secrets_test.yaml`:
- Around line 2-6: The suite-level templates list in
charts/ctrlplane/tests/secrets_test.yaml is missing
charts/web/templates/deployment.yaml and
charts/pty-proxy/templates/deployment.yaml, so add those two entries to the
top-level templates: array; then add test jobs that target those templates to
assert the env wiring: for charts/web/templates/deployment.yaml add tests
asserting container env contains name: AUTH_SECRET bound via
valueFrom.secretKeyRef to the auto-generated ctrlplane-web/AUTH_SECRET and a
second test that sets global.secrets.authSecret.valueFrom (e.g., name:
my-ext-auth, key: MY_AUTH) and asserts the ARTIFACT uses that secret; for
charts/pty-proxy/templates/deployment.yaml add tests asserting
VARIABLES_AES_256_KEY is wired to ctrlplane-encryption-key/AES_256_KEY and a
counterpart that sets global.secrets.encryptionKey.valueFrom (e.g.,
my-ext-aes/MY_AES) and asserts the valueFrom binding is used.
| {{- if and $existing (hasKey $existing.data "AUTH_SECRET") }} | ||
| AUTH_SECRET: {{ index $existing.data "AUTH_SECRET" }} | ||
| {{- else }} | ||
| AUTH_SECRET: {{ randAlphaNum 64 | b64enc }} | ||
| {{- end }} No newline at end of file | ||
| {{- end }} |
There was a problem hiding this comment.
Secret reuse and generation logic is correct; minor style inconsistency with | quote.
index $existing.data "AUTH_SECRET" correctly returns the already-base64-encoded value from the live Secret, safe to assign directly to data:. The short-circuit and safely handles the nil case when lookup returns nothing (e.g., first install or --dry-run).
Minor: {{ randAlphaNum 64 | b64enc }} (line 14) omits | quote, while charts/ctrlplane/templates/secrets.yaml uses {{ randAlphaNum 64 | b64enc | quote }}. Both produce valid YAML since base64 is YAML-safe, but aligning would improve consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/ctrlplane/charts/web/templates/secrets.yaml` around lines 11 - 15, The
AUTH_SECRET generation is inconsistent: when reusing an existing Secret you
assign the base64 value via index $existing.data "AUTH_SECRET", but the
newly-generated branch uses {{ randAlphaNum 64 | b64enc }} without | quote;
update the generated branch to mirror the existing-template style by piping the
b64 output through | quote (i.e., change the randAlphaNum 64 | b64enc branch to
include | quote) so both branches produce consistent YAML for AUTH_SECRET.
2 participants
Background
The chart right now does not support various ingress configurations, like adding hostname and tls. Additionally, when using a gitops pattern, folks need the ability to bring their own secrets as opposed to being rendered on the fly. Namely,
AUTH_SECRETandAES_256_KEYshould have the option of byos (bring your own secret).What's Changed?
Summary by CodeRabbit